fix(datastore): retry dropped endpoint registrations instead of silently untracking pods#2092
Conversation
…d reconciles An upsert that overlaps an in-flight delete (or whose collector fails to start) observes a nil endpoint from NewEndpoint with no surviving pods-map entry. Previously this was silently swallowed: the pod stayed untracked until the next pod event, which can leave the flow control dispatch cycle with an empty candidate list and saturation pinned at 1.0. upsertEndpoint now distinguishes the benign duplicate-start race (a concurrent upsert already stored the entry) from a dropped registration, which is returned as an error. PodReconciler propagates it so controller-runtime requeues with backoff; podResyncAll propagates it through PoolSet for the same effect on the pool reconciler; discovery sources log it. Related to llm-d#2060. Signed-off-by: Luke Van Drie <lukevandrie@google.com>
…ta on upsert races Close gaps in the dropped-registration handling: - PoolSet stores the pool before resyncing, so a PoolSet retried after a resync failure compared the incoming pool against the already-stored identical pool and skipped the resync entirely. A needsResync flag now forces the retried PoolSet to resync until one succeeds. - upsertEndpoint applies the caller's metadata through the update path when a concurrent upsert wins the registration race, instead of silently discarding it. - The dropped-registration error is a sentinel (errRegistrationDropped) so callers and tests can match it with errors.Is. - PodUpdateOrAddIfNotExist skips and logs at DEBUG when the pool is not synced instead of logging a misleading 'Pod already exists'; the added/exists logging lives in one place. - Unchecked calls of the now error-returning PodUpdateOrAddIfNotExist are handled or explicitly discarded to satisfy errcheck. - The bespoke test endpoint factories collapse into a configurable datalayer.FakeEndpointFactory. Related to llm-d#2060. Signed-off-by: Luke Van Drie <lukevandrie@google.com>
Signed-off-by: Luke Van Drie <lukevandrie@google.com>
Signed-off-by: Luke Van Drie <lukevandrie@google.com>
dc6fcda to
d137241
Compare
ahg-g
left a comment
There was a problem hiding this comment.
Why not address the race condition itself? I am surprised that we don't do that for all things related to pod and endpoint updates to the datastore.
Mostly because the nil from Fixing the race at the source is also less simple than it looks. We probably need per-key serialization in the collector registry plus a typed error from With the retry, losing the race costs one requeue instead of a permanently missing endpoint. If you have any other ideas here that are less complex, please let me know. To my best knowledge this race window is very small and unlikely to be hit in practice. |
|
/approve |
What type of PR is this?
/kind bug
What this PR does / why we need it:
An endpoint upsert that overlaps an in-flight delete observes
Runtime.NewEndpointreturning nil (the previous collector is still registered) while the pods-map entry is already gone. The datastore skipped storing the pod and the reconciler reported success, so the pod stayed untracked until the next pod event; for a stable Ready pod that event may never come. An empty datastore pods map makes the flow control saturation detectors fail closed, pinningflow_control_pool_saturationat 1.0 and halting dispatch (#2060).sequenceDiagram participant Del as Delete path (PodDelete / resync) participant DS as Datastore participant R as Runtime (collector registry) participant Rec as PodReconciler (upsert) Del->>DS: PodDelete(pod) DS->>DS: pods.Delete(key) DS->>R: ReleaseEndpoint(ep) Note over R: dispatches EventDelete to extractors<br/>(collector still registered) Rec->>DS: PodUpdateOrAddIfNotExist(pod) DS->>DS: pods.Load(key): miss DS->>R: NewEndpoint(meta) R-->>DS: nil (collector still registered) DS->>DS: pods.Load(key): still miss DS-->>Rec: errRegistrationDropped Note over Rec: before this PR: nil swallowed, reconcile "succeeds",<br/>pod untracked until the next pod event.<br/>Now: error returned, controller-runtime requeues with backoff. R->>R: collectors.Remove(key), Stop() Rec->>DS: retry: PodUpdateOrAddIfNotExist(pod) DS->>R: NewEndpoint(meta) R-->>DS: endpoint DS->>DS: pods.Store(key, ep) DS-->>Rec: nil (tracked)This PR makes the drop detectable and retryable:
upsertEndpointdistinguishes the benign duplicate-start race (a concurrent upsert stored the entry; this call's metadata is applied through the update path) from a dropped registration, which is returned as an error wrapping a sentinel.PodReconcilerpropagates the error so controller-runtime requeues with backoff.podResyncAllpropagates the error throughPoolSet, and aneedsResyncflag makes the retriedPoolSetre-run the resync (the pool is stored before resyncing, so the pool comparison alone would skip the retry).needsResyncflag).Follow-up, deliberately not in this PR: have
Runtime.NewEndpointreturn a typed error distinguishing "collector already registered" from "collector failed to start", and evaluate closing the window inReleaseEndpointitself. The current release ordering (dispatch the delete event, then deregister the collector) is critical for extractor state, so a naive reorder is not safe.Which issue(s) this PR fixes:
Related to #2060. Closure is gated on a multi-replica re-test against main; see the issue thread.
Release note: